Skip to content

fs: cancel in-flight stat on abort - #63142

Open
mertcanaltin wants to merge 1 commit into
nodejs:mainfrom
mertcanaltin:mert/fs-stat-inflight-fix
Open

fs: cancel in-flight stat on abort#63142
mertcanaltin wants to merge 1 commit into
nodejs:mainfrom
mertcanaltin:mert/fs-stat-inflight-fix

Conversation

@mertcanaltin

@mertcanaltin mertcanaltin commented May 6, 2026

Copy link
Copy Markdown
Member

The AbortError behavior was landed in main with #63143, leaving only the uv_cancel call that actually dropped the request from the threadpool queue in this PR.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 6, 2026
@mertcanaltin
mertcanaltin force-pushed the mert/fs-stat-inflight-fix branch from 4425502 to e4d317e Compare May 6, 2026 05:18
@bakkot

bakkot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Drive-by: this looks like it's still not actually canceling the underlying call, which would involve invoking uv_cancel somewhere.

@mertcanaltin
mertcanaltin force-pushed the mert/fs-stat-inflight-fix branch from e4d317e to 8149c79 Compare May 7, 2026 19:42
@codecov

codecov Bot commented May 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (46a7dbd) to head (2d21670).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63142      +/-   ##
==========================================
- Coverage   90.21%   90.16%   -0.05%     
==========================================
  Files         751      751              
  Lines      253550   253557       +7     
  Branches    47813    47772      -41     
==========================================
- Hits       228733   228623     -110     
- Misses      16076    16193     +117     
  Partials     8741     8741              
Files with missing lines Coverage Δ
lib/fs.js 98.43% <100.00%> (+<0.01%) ⬆️
src/node_file.cc 74.15% <83.33%> (+0.01%) ⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mertcanaltin
mertcanaltin force-pushed the mert/fs-stat-inflight-fix branch 2 times, most recently from fc73958 to 6d3d629 Compare May 8, 2026 20:30
@mertcanaltin

Copy link
Copy Markdown
Member Author

Thanks for review @bakkot I added a new binding function for cancel

@trivikr

trivikr commented Jul 16, 2026

Copy link
Copy Markdown
Member

cc @nodejs/fs

@mertcanaltin

Copy link
Copy Markdown
Member Author

@Renegade334, could you review this pr?

@mertcanaltin
mertcanaltin force-pushed the mert/fs-stat-inflight-fix branch from 6d3d629 to 4913c16 Compare August 20, 2026 10:26
@MikeMcC399 MikeMcC399 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@MikeMcC399 MikeMcC399 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mertcanaltin

Copy link
Copy Markdown
Member Author

I measuring uv_cancel.

@trivikr

trivikr commented Aug 24, 2026

Copy link
Copy Markdown
Member

@mertcanaltin Merge commits don't work with Node.js tooling. Can you please rebase?

Signed-off-by: Mert Can Altin <mertgold60@gmail.com>
@mertcanaltin
mertcanaltin force-pushed the mert/fs-stat-inflight-fix branch from 4eb1ce8 to 2d21670 Compare August 24, 2026 14:25
@mertcanaltin

mertcanaltin commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

Thanks @trivikr, I've rebased now and measured.

UV_THREADPOOL_SIZE=1, pool blocked by a pbkdf2 job, all 10,000 stadiums that were aborted while the pool was clogged are actually cancelled.

executed canceled settled
without cancel 10000 0 528 ms
with cancel 0 10000 14 ms

@mertcanaltin mertcanaltin changed the title fs: abort in-flight stat operations fs: cancel in-flight stat on abort Aug 24, 2026
@mertcanaltin

Copy link
Copy Markdown
Member Author

Update: The AbortError behavior was landed in main with #63143, leaving only the uv_cancel call that actually dropped the request from the threadpool queue in this PR.

@mertcanaltin
mertcanaltin requested a review from jasnell August 24, 2026 14:42
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants